Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add option to transpose writeback connector by swapping w/h on fb #3

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

6by9
Copy link
Contributor

@6by9 6by9 commented Aug 14, 2024

For your comments

Goes alongside raspberrypi/linux#6312

@6by9
Copy link
Contributor Author

6by9 commented Aug 14, 2024

@cillian64
@popcornmix
As discussed yesterday.
I haven't added an actual property as the width/height are already validated, so other than square output images the kernel can detect that a transpose is desired.

Admittedly not having a property or capability makes it more tricky for userspace to know that the writeback connector can transpose.

@cillian64
Copy link

I have no objections in principle. I'm not too worried about square images, I expect it's very uncommon and in most cases we could pad it to be rectangular then crop off the padding after transposing.

For the capability, drmu could have a helper method to try a transpose and report if it worked (in the same way it currently does a trial writeback to check if it works).

@jc-kynesim
Copy link
Owner

I'm strongly in favour of a property on the connector that sets transpose rather than an implicit operation. Simple presence/ absence could indicate capability though probably an enum where the available value(s) indicate support c.f. the rotation property on planes.

@6by9
Copy link
Contributor Author

6by9 commented Aug 23, 2024

I'm strongly in favour of a property on the connector that sets transpose rather than an implicit operation. Simple presence/ absence could indicate capability though probably an enum where the available value(s) indicate support c.f. the rotation property on planes.

Agreed - it was just the amount of boilerplate required to add a property wasn't worth it for a PoC.

Annoyingly the DRM property for plane rotation supports rotation (0/90/180/270) and reflect X/Y, so there's no way to just say transpose if it was reused.

@jc-kynesim
Copy link
Owner

Doesn't (Rotate-90 | Reflect-X) get you transpose (the property is a bitfield of enums I believe)? As I remember if you want a Reflect you have to combine it with a Rotate so that should work? (I recall spending a while wondering why Reflect wasn't working for me till I combined it with Rotate-0.) Though that has the unfortunate property of implying support for Rotate-90 & Reflect-X individually when enumerating the enum so maybe not a great idea.

@6by9
Copy link
Contributor Author

6by9 commented Aug 23, 2024

Though that has the unfortunate property of implying support for Rotate-90 & Reflect-X individually when enumerating the enum so maybe not a great idea.

That's the problem - I can't say that ONLY transpose is valid.

There is drm_rotation_simplify to simplify rotate 180 back into REFLECT_X | REFLECT_Y, so it would be nice to extend to have it also be an option for transpose so ROTATE_90 becomes TRANSPOSE | REFLECT_X.
(NB it must be defined whether the transpose is before or after the reflection as it is noncommutative. I guess that may be why they kept it as rotation as then you can convert that regardless of the operations in hardware)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants